CORE-2006: Migrate ProfileMenu and HelpMenu components to plain CSS - #141
CORE-2006: Migrate ProfileMenu and HelpMenu components to plain CSS#141OpenStaxClaude wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates ProfileMenu and HelpMenu from styled-components to plain CSS while retaining theme-driven values through CSS custom properties.
Changes:
- Adds plain CSS styling for both menus.
- Replaces styled wrappers with class-based React components.
- Stabilizes the ProfileMenu focus snapshot.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ProfileMenu/ProfileMenu.css |
Defines ProfileMenu styles. |
ProfileMenu/index.tsx |
Adds class-based wrappers and theme variables. |
ProfileMenu/index.spec.tsx |
Waits for settled focus state. |
ProfileMenu/__snapshots__/index.spec.tsx.snap |
Updates rendered output. |
HelpMenu/HelpMenu.css |
Defines HelpMenu and iframe styles. |
HelpMenu/index.tsx |
Replaces styled wrappers and iframe components. |
HelpMenu/__snapshots__/index.spec.tsx.snap |
Updates rendered output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
5b92dd5 to
0dcff07
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0dcff07 to
937d2da
Compare
|
@RoyEJohnson understood — unstacked and paused. Unstacked. The branch is rebased back onto What that costs, so the pause is a knowing one. Composing So this round keeps what works against Still paused and still a draft. The remaining two gaps are a small commit each once #136 and #137 are on Two open questions whenever you get to them, no rush: the 14px profile-popover position (that one changes what ships), and whether you'd rather I fold the |
61e5db8 to
27ab430
Compare
937d2da to
2f6f046
Compare
27ab430 to
e18e5ec
Compare
2f6f046 to
2b4ee85
Compare
e18e5ec to
590bda0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2b4ee85 to
5d4e885
Compare
This comment was marked as resolved.
This comment was marked as resolved.
590bda0 to
9a9bae3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
f467ea4 to
83dd3fe
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f9cf833 to
5f4d6f9
Compare
83dd3fe to
7993af3
Compare
Establishes one place a theme value is written and referenced from CSS, with
a test that fails if the two disagree. The sweep of the already-migrated
stylesheets onto it follows separately.
src/theme/theme.css holds a single :root block. Colour tokens are the
kebab-case palette key (palette.neutralLighter -> --ox-color-neutral-lighter)
plus --ox-color-link, --ox-color-link-hover, --ox-z-index-* and
--ox-padding-navbar-*. The --ox- prefix avoids collisions with a consuming
app's own variables.
The file is generated, not hand-written. themeCss.ts owns the projection and
npm run generate:theme-css writes it; build.bash runs it as its first step,
before either tsc pass and before the rsync, and publish.bash inherits that
via build:clean, so a published package cannot ship a stale file. It is
committed as well as generated because jest and ladle read src/ directly and
CI runs lint/test rather than build. Deliberately not hooked into pretest —
regenerating before the suite would make the freshness check pass vacuously.
Adds the four button variant colours to the palette, which theme/buttons.ts
had been holding as bare string literals with nothing recording that they
are hover/active variants of orange and darkGray. Purely additive.
Enforcement, in tokens.spec.ts, on top of the CORE-2736 engine:
1. The committed theme.css is exactly what the generator produces. One
equality, so a missing token, an orphan token and a stale value all
fail the same way.
2. No component stylesheet writes a colour literal that duplicates a
theme value.
3. No component stylesheet introduces a colour that is neither a theme
value nor on the KNOWN_OFF_PALETTE allowlist, each entry with a reason.
4. No component stylesheet reads an --ox-* token that does not exist,
which would otherwise fall through to its fallback silently.
Check 2 cannot pass yet — 16 stylesheets migrated before the tokens existed
still carry hand-copied literals. PENDING_SWEEP names them, and is asserted
to be exactly the failing set so it cannot rot in either direction: dropping
a name without sweeping the file fails, and sweeping a file without dropping
its name fails too. The list reaches empty in the sweep PR and goes away
with the assertion.
No CSS @import: build.bash rsyncs CSS 1:1 with no bundler, so an @import
would depend on each consumer's resolver. Component .tsx files import
theme.css alongside their own stylesheet instead. Consumers need do nothing.
Split out of #143. Stacked on #149.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "what CI enforces" list read as though the duplicate-literal check covered every stylesheet, which it will not until the sweep lands. Says which files are exempt, why the list cannot drift, and that new stylesheets are not to be added to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4424b6f to
1a50ffa
Compare
Replaces the styled-components definitions in ProfileMenu/index.tsx and HelpMenu/index.tsx with plain CSS files, following the hybrid approach used by the other components in this migration: theme values stay in JavaScript and are bound to CSS custom properties on the element, with the palette hex as a fallback in the CSS. No test changes: the snapshot flake this branch originally worked around is gone from main, CORE-2715 (#138) and CORE-2716 (#139) having replaced both component snapshots with targeted assertions. Those specs pass against the migrated components unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton merged the caller's style into their CSS-variable object with a spread. react-aria-components types style as `CSSProperties | ((renderProps) => CSSProperties)`, and spreading a function copies nothing, so a render-callback style was silently dropped and replaced by the wrapper's static object. Both now build their variables inside composeRenderProps, the same shape CORE-2710 (#137) uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads last, and the object form is unchanged -- all 116 snapshots pass untouched. Their style reaches react-aria's Button directly (HelpMenuButton's by way of NavBarButton, which passes it straight through), so this works against main as it stands. ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for now, with a comment saying why: they hand style to NavBarMenuItem, which spreads it as well, so composing only here would send a function into that spread and lose these variables along with the caller's style. Both levels have to compose together, which needs #137 on main first. Eight tests: three per button wrapper (render-callback merged, render-callback overriding the wrapper variables, object form merging caller-last) plus object-form coverage for each menu item. Confirmed failing before the fix: stashing only the two component files leaves 4 failed, 33 passed across the two specs -- the four callback and override cases fail, the object-form cases pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and NavBarPopover compose className through composeRenderProps. The wrappers added here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem, HelpMenuButton and HelpMenuItem passed the caller's className straight into classNames(), which ignores functions, so a render-callback className was silently dropped -- and unlike the style case TypeScript does not catch it. Composing only at this level was not possible before #136: the composed function would have reached NavBarMenuItem's classNames() and been discarded there, taking the profile-menu-item / help-menu-item class with it. Two tests per component, matching the shape #136 uses: a render-callback className reaches the DOM alongside the wrapper's own class, and a string className still composes. Confirmed failing before the fix: stashing only the two component files leaves the two render-callback cases red. The style half of this is still split. The two button wrappers compose it; the two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem still spreads style. Comments in both files say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows the CORE-2720 (#143) sweep, which this branch is now stacked on. The two new stylesheets repeated eleven palette hexes as var() fallbacks; they now read the --ox-* tokens instead, e.g. color: var(--help-menu-button-color, var(--ox-color-gray)); The override hooks are unchanged -- only their defaults moved from JavaScript to the CSS side, so the components no longer bind static custom properties inline. That means style is no longer destructured in ProfileMenuButton, ProfileMenuItem, HelpMenuButton or HelpMenuItem: it passes through in ...props and react-aria handles both the object and render-callback forms itself. Two consequences worth naming: - The CORE-2710 (#137) dependency is gone rather than deferred. The bug it guards against was a wrapper overwriting the caller's style, which these wrappers no longer do, so the menu items need nothing from #137. Same reasoning as the note #143 leaves on NavBarMenuItem. - iframeWrapperStyle and putAwayStyle are gone; the iframe wrapper and the put-away bar take their colours from HelpMenu.css. className composition stays -- that one is a real bug fix, not a default. The specs that asserted the inline defaults now assert what matters instead: the caller's style reaches the element in both forms, and the override hook still wins. Defaults are covered centrally by src/theme/tokens.spec.ts, which also fails on any colour literal that duplicates a theme value -- both new stylesheets pass it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7993af3 to
a075964
Compare
|
Rebased — base changed to #150, history rewritten. Heads-up since this rewrites a branch you may have checked out: Roy asked for #143 to be split for review, so it is now This PR moved to #150 rather than #143 because it does not need the sweep — it has zero file overlap with it. That means it is no longer waiting on sweep review to land. Rebased 481 tests / 41 suites / 114 snapshots pass; typecheck clean. No snapshot regeneration needed, which is the check that this PR really was independent of the sweep — if it had been rendering a component whose inline defaults the sweep removes, its snapshots would have shifted. 🤖 Generated with Claude Code |
f74f4fe to
8ed36c2
Compare
Jira: CORE-2006 (subtask of CORE-1777)
Migrates
ProfileMenu/index.tsxandHelpMenu/index.tsxoff styled-components, following the same hybrid pattern as the already-merged subtasks (#130 NavBar, #131 ManageCookies): theme values stay in JavaScript and are bound to CSS custom properties on the element, with the palette hex inlined as the CSS fallback.Review round 6 — history linearised onto the current #143 head
The branch had picked up a merge commit: #143 rebased (onto
main, so it now carries #138 and #137), and its new head was merged into this branch rather than this branch being rebased onto it. That left both the old and the new copies of #143's commits in the history. Reset to #143's current head and replayed only this PR's four commits, so the history is linear again and the diff is exactly the six files below. The #138 cherry-pick this PR used to carry is gone too — #143 contains #138 now, so it is redundant.On the base itself — it is
CORE-2720-global-css-theme-tokensbecause of the round-5 request to stack on #143 and use the global tokens, and it stays there unless you say otherwise. Flipping tomainis one command on my side, but it would mean dropping the token-sweep commit and going back to palette hexes as thevar()fallbacks until #143 lands, sincetheme.cssand the--ox-*tokens do not exist onmain. That shape was green two rounds ago, so it is a cheap revert — just say which you'd rather have.Review round 5 — stacked on #143 and moved onto the global tokens
Done as asked: base is
CORE-2720-global-css-theme-tokens, and the two new stylesheets read--ox-*tokens instead of repeating palette hexes —var(--help-menu-button-color, var(--ox-color-gray))rather thanvar(--help-menu-button-color, #5e5e5e). Eleven fallbacks in total.src/theme/tokens.spec.tspasses on both files, so they're now covered by the same guard as the rest of the library.The override hooks are unchanged; only their defaults moved from JavaScript to CSS, exactly as #143 did elsewhere. So these components no longer bind static custom properties inline,
styleis no longer destructured in any of the four wrappers, and it passes straight through in...props.That removes the #137 dependency rather than deferring it. The bug CORE-2710 guards against is a wrapper overwriting the caller's
style— which these wrappers no longer do.ProfileMenuItem/HelpMenuItemneed nothing from #137 now, and thecomposeRenderPropswrapping I'd added for the two buttons is gone with the defaults it existed to protect. Same reasoning as the note #143 leaves onNavBarMenuItem.classNamecomposition stays: that one is a real bug fix, not a default.iframeWrapperStyleandputAwayStyleare gone too — the contact-form iframe wrapper and the put-away bar take their colours fromHelpMenu.css.One thing carried deliberately. This base predates #138, which is already on
main. Without it the old flaky ProfileMenu snapshot fails against the migrated component, so #138 is cherry-picked here (c7ff9ac, authorship preserved) and shows in the diff below as the snapshot deletion plus the targeted-assertion spec. It drops out as a duplicate as soon as #143 rebases ontomain. Happy to drop it sooner if you'd rather rebase #143 first.Changes
ProfileMenu— newProfileMenu/ProfileMenu.cssProfileMenuButton(styled(Button)) →forwardRefwrapper over react-ariaButton, classprofile-menu-button. Colours arevar(--profile-menu-button-color, var(--ox-color-white))/var(--profile-menu-button-bg, var(--ox-color-dark-teal))in the CSS — the hooks are still there for consumers, the defaults just aren't set inline. ThedefaultFocusOutlinefragment is inlined under[data-focused], [data-focus-visible], matching howNavBarMenuButtons.csshandles it.ProfileMenuPopover(styled(NavBarPopover)) →forwardRefwrapper, classprofile-menu-popover.ProfileMenuItem(styled(NavBarMenuItem)) →forwardRefwrapper, classprofile-menu-item;var(--profile-menu-item-color, var(--ox-color-neutral-darker))in the CSS.HelpMenu— newHelpMenu/HelpMenu.cssHelpMenuButton(styled(NavBarMenuButton)) → function component, classhelp-menu-button;var(--help-menu-button-color, var(--ox-color-gray))in the CSS.HelpMenuItem(styled(NavBarMenuItem)) →forwardRefwrapper, classhelp-menu-item;var(--help-menu-item-color, var(--ox-color-neutral-darker))andvar(--help-menu-item-focus-bg, var(--ox-color-neutral-lighter))in the CSS.IframeWrapper(styled(BodyPortal)) andIframe(styled.iframe) →BodyPortal+ plainiframewithhelp-menu-iframe-wrapper/help-menu-iframe; their colours come from the stylesheet, so no inline style is passed at all.StyledPutAway(styled(PutAway)) → the internalPutAwaynow carrieshelp-menu-put-awaydirectly; the@media(min-width: 56em)block is a standard media query.PutAwaykeeps itsclassNameprop.NewTabIconis untouched — itsfill={colors.palette.neutralThin}was already plain JS, not styled-components.Public API is unchanged: every exported name still exists, still accepts
className/style(so a consumer'sstyled(...)extension keeps working), andProfileMenuButton/ProfileMenuItem/HelpMenuItemforward refs exactly as the styled versions did.While screenshotting the Ladle stories I found the compound-specificity question above is not academic. On
mainthe ProfileMenu popover overlaps the trigger button; before the NavBar migration it sat just below it.Mechanism: pre-#130,
ProfileMenuPopover = styled(NavBarPopover)extended a styled component, so styled-components guaranteed the child'smargin-top: 0.4remwas injected after the parent'smargin-top: -1remand won. #130 turnedNavBarPopoverinto a plain component with.navbar-popover { margin-top: -1rem }in a real stylesheet.ProfileMenuPopover's styled-components class then tied on specificity and lost on injection order — Vite's stylesheet wins over the styled-components runtime sheet in this build. So the 0.4rem override has been silently dead onmainsince #130, and the popover moved up 1.4rem.Verified with Chromium screenshots of the Ladle stories at three refs —
33f379f9c(last commit before #130),main, and this branch:profile-menu--with-initialsopen / hoverprofile-menu--with-linksopen / hoverprofile-menu--with-user-icon-fallbackopen / hoverprofile-menu--standaloneopen / hoverprofile-menuclosed stateshelp-menu--defaultclosed / open / hoverSo this branch is a byte-for-byte match with the pre-migration styled-components rendering, and
mainis the outlier. I took that as the intended target, since the parent ticket's success criterion is no visual regression against the styled-components baseline and nothing in #130 suggests the shift was deliberate. But it does mean this PR visibly moves the profile popover down 14px relative tomain— if you'd rather keep what's currently deployed, drop themargin-top: 0.4remline and I'll re-baseline.HelpMenuItem's:focus-visibleoverride (outline: 0+ background) is the same story by the same mechanism — it is equally dead onmainand equally restored here. That one I did not manage to capture in a screenshot; forcing Chromium's:focus-visibleon a programmatically focused menu item defeated me, so it is an inference from the popover's demonstrated ordering rather than a measurement.Two things worth a reviewer's eye
1. Compound selectors for the two real overrides. styled-components resolved
.navbar-popovervsProfileMenuPopoverand.navbar-menu-item:focus-visiblevsHelpMenuItem:focus-visibleby injection order (equal specificity, later class wins). In plain CSS that would depend on stylesheet order, so those two rules are written as.navbar-popover.profile-menu-popoverand.navbar-menu-item.help-menu-item:focus-visible— one extra class of specificity makes the outcome order-independent. Both are commented in the CSS. Everything else is a single-class selector.2. The test diff is additive plus one carried commit. #138 and #139 replaced both components' snapshots with targeted assertions, and this branch leaves both specs exactly as they stand — it only appends
describeblocks for thestylepassthrough andclassNamecomposition. Those new blocks assert the caller'sstylereaches the element and the override hook wins, rather than asserting inline defaults, matching the reconciliation #143 made to the CORE-2710 tests. The only non-additive part is the #138 cherry-pick described above, which isn't really this PR's change.Verification
npm test— 40 suites / 388 tests pass, 114 snapshots, no snapshot updates (the count includes CORE-2720: Sweep the migrated CSS onto the theme tokens #143's suite, since that's the base).tokens.spec.tspasses on both new stylesheets.npm run lint— cleannpx tsc --noEmit— cleannpm run build— succeeds, andProfileMenu.css/HelpMenu.cssare rsynced intodist/esmanddist/cjsmainand the last pre-CORE-2004: Migrate NavBar components to plain CSS #130 commitNot in this PR
ProfileMenu.stories.tsxandHelpMenu.stories.tsxstill usecreateGlobalStylefor the body-portal nav positioning. Story files belong to CORE-2009, and CORE-2005: Migrate navigation components (SidebarNav, ButtonNav, Pagination) to plain CSS #140 leftSidebarNav.stories.tsxthe same way, so I kept the boundary. Say the word if you'd rather absorb them here.🤖 Generated with Claude Code